fix(cli): prevent TTY hang during command delegation (#1396)#1422
fix(cli): prevent TTY hang during command delegation (#1396)#1422Han5991 wants to merge 7 commits intovoidzero-dev:mainfrom
Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
Han5991
force-pushed
the
fix/1396-lefthook-tty-hang
branch
from
3dc99eb to
e965560
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a5ccb10a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
@Han5991 The amount of changes currently is a bit too large. Why wasn't the solution mentioned here #1396 (comment) considered? |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
You're right that the earlier change set was broader than it needed to be. I did try the Option 1 approach from #1396 by adding the foreground-process-group check in I also should have evaluated Option 2 from that comment earlier. I did not give that path I've since dropped those CLI-side header workarounds and kept the fix localized to The current change uses the Option 2-style approach instead: the terminal color query now |
…r header evaluation (voidzero-dev#1396) Commands like `vp run` or `vp test` running under background/reporter environments (e.g., lefthook with certain Node.js versions) would hang due to `query_terminal_colors()` blocking on `/dev/tty`. The root cause was `apply_custom_help()` eagerly evaluating the `vite_plus_header()` (and its OSC queries) to build the clap help template during argument parsing, prior to command dispatch. This commit: - Removes the eager header evaluation from clap help templates. - Prints the dynamic `vite_plus_header()` lazily only on `ErrorKind::DisplayHelp`. - Explicitly passes `dynamic_colors` flags into `print_runtime_header()`, allowing delegate commands to safely use `vite_plus_header_static()` right before execution without blocking on TTY queries.
Han5991
force-pushed
the
fix/1396-lefthook-tty-hang
branch
from
c5a03ba to
22978f1
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22978f1a5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rx.recv_timeout(Duration::from_millis(QUERY_TERMINAL_COLORS_WALL_CLOCK_TIMEOUT_MS)) | ||
| .unwrap_or((None, vec![])) |
There was a problem hiding this comment.
query_terminal_colors now returns after a fixed 250ms timeout, but the spawned query_terminal_colors_inner path can keep /dev/tty in raw mode for up to 1000ms when SSH_CONNECTION/SSH_TTY is set. If the outer timeout fires first, the caller continues while the detached thread still owns terminal mode changes, so delegated/interactive commands can start with a raw tty and exhibit input/editing glitches or apparent hangs. This regression is introduced by the new async wrapper and should be fixed by making the outer timeout cover the inner worst-case window (or by preventing raw-mode mutation in a detached worker).
Useful? React with 👍 / 👎.
|
@Han5991 Determining terminal color support through I feel that the current fix is not good. Starting a thread to determine terminal color always feels a bit odd. |
|
I think keeping the OSC query with a bounded fallback is still reasonable, but the detached worker-thread timeout is not the right shape. The safer approach would be:
That preserves the intended behavior: use terminal-specific colors when they can be queried safely, otherwise fall back to the default gradient. It also avoids the regression risk introduced by letting command |
2 participants
Resolves #1396
Description
Commands like
vp runorvp testrunning under background/reporter environments (e.g., lefthook with certain Node.js versions) would hang due toquery_terminal_colors()blocking on/dev/tty.The root cause was
apply_custom_help()eagerly evaluating thevite_plus_header()(and its OSC queries) to build the clap help template during argument parsing, prior to command dispatch.This commit:
vite_plus_header()lazily only onErrorKind::DisplayHelp.dynamic_colorsflags intoprint_runtime_header(), allowing delegate commands to safely usevite_plus_header_static()right before execution without blocking on TTY queries.